add files to compute basic stats on pseudo crawl dataset#382
Open
SaulLu wants to merge 8 commits intobigscience-workshop:masterfrom
Open
add files to compute basic stats on pseudo crawl dataset#382SaulLu wants to merge 8 commits intobigscience-workshop:masterfrom
SaulLu wants to merge 8 commits intobigscience-workshop:masterfrom
Conversation
SaulLu
force-pushed
the
compute-statistics
branch
from
96fcc41 to
19c004e
Compare
for more information, see https://pre-commit.ci
thomasw21
approved these changes
Feb 4, 2022
Member
thomasw21
left a comment
thomasw21
left a comment
There was a problem hiding this comment.
LGTM! A few comments but should be good.
| parser.add_argument( | ||
| "--save-batch-size", type=int, required=True, help="Batch size when writing." | ||
| ) | ||
| parser.add_argument("--use-datasets-caching", action="store_true") |
Member
There was a problem hiding this comment.
We never us it. Let's remove it.
Contributor
Author
There was a problem hiding this comment.
In fact, it seems to me that I used use_datasets_caching
Contributor
Author
There was a problem hiding this comment.
I'll removed save-batch-size
| "--save-path-stats-json", type=str, help="Where to save the stats json." | ||
| ) | ||
| parser.add_argument( | ||
| "--save-path-stats-full-json", type=str, help="Where to save the stats json." |
Member
There was a problem hiding this comment.
Suggested change
| "--save-path-stats-full-json", type=str, help="Where to save the stats json." | |
| "--save-path-stats-full-json", type=str, required=True, help="Where to save the stats json." |
Comment on lines
+71
to
+77
| logger.info(f" --- Statistics not already computed for seed id {args.seed_id} ") | ||
| if not args.use_datasets_caching: | ||
| datasets.set_caching_enabled(False) | ||
| else: | ||
| logger.info( | ||
| f"the datasets results will be cached at {config.HF_DATASETS_CACHE}." | ||
| ) |
Member
There was a problem hiding this comment.
Suggested change
| logger.info(f" --- Statistics not already computed for seed id {args.seed_id} ") | |
| if not args.use_datasets_caching: | |
| datasets.set_caching_enabled(False) | |
| else: | |
| logger.info( | |
| f"the datasets results will be cached at {config.HF_DATASETS_CACHE}." | |
| ) | |
| logger.info(f" --- Statistics not already computed for seed id {args.seed_id} ") |
Contributor
Author
There was a problem hiding this comment.
In fact, it seems to me that I used use_datasets_caching
| ] | ||
| ds_html = ds_html.map( | ||
| get_length_text, | ||
| batched=False, |
Member
There was a problem hiding this comment.
Suggested change
| batched=False, | |
| batched=True, |
You just have to code a batched version of get_length_text
| with open(save_path_tmp, "w", encoding="utf-8") as f: | ||
| json.dump(data_stats, f, ensure_ascii=False, indent=4) | ||
| logger.info(f"Moving the saved dataset to {str(save_path.absolute())}") | ||
| subprocess.run(["mv", save_path_tmp, str(save_path.absolute())]) |
Member
There was a problem hiding this comment.
Yeah I'd recomment os.rename. subprocess doesn't tell you if it fails.
Comment on lines
+151
to
+161
| save_path = Path(args.save_path_stats_full_json) | ||
| tmp_file_name = f"tmp-{str(save_path.name)}" | ||
| save_path_tmp = os.path.join(save_path.parent, tmp_file_name) | ||
| logger.info(f"Saving the dataset at {save_path_tmp}") | ||
| ds_html.to_json( | ||
| save_path_tmp, | ||
| batch_size=args.save_batch_size, | ||
| num_proc=args.num_proc, | ||
| compression="gzip", | ||
| ) | ||
| logger.info(f"Moving the saved dataset to {str(save_path.absolute())}") |
Member
There was a problem hiding this comment.
Suggested change
| save_path = Path(args.save_path_stats_full_json) | |
| tmp_file_name = f"tmp-{str(save_path.name)}" | |
| save_path_tmp = os.path.join(save_path.parent, tmp_file_name) | |
| logger.info(f"Saving the dataset at {save_path_tmp}") | |
| ds_html.to_json( | |
| save_path_tmp, | |
| batch_size=args.save_batch_size, | |
| num_proc=args.num_proc, | |
| compression="gzip", | |
| ) | |
| logger.info(f"Moving the saved dataset to {str(save_path.absolute())}") | |
| save_path_full = Path(args.save_path_stats_full_json) | |
| save_path_full_tmp = save_path_full.rename(f"{save_path_full.name}.tmp") | |
| logger.info(f"Saving the dataset at {save_path_full_tmp}") | |
| ds_html.to_json( | |
| save_path_full_tmp, | |
| batch_size=args.save_batch_size, | |
| num_proc=args.num_proc, | |
| compression="gzip", | |
| ) | |
| logger.info(f"Moving the saved dataset to {str(save_path_full.absolute())}") |
two things:
- move with
os.rename, orshutil.move - You're not scared of overiting previous full_json?
| #SBATCH --partition=cpu_p1 | ||
| #SBATCH --time 10:00:00 # maximum execution time (HH:MM:SS) | ||
| #SBATCH --output=/gpfswork/rech/six/uty16tp/code/big_science/logs/compute_stats_v5/%x-%j.out # output file name | ||
| #SBATCH --array=1-604 |
Member
There was a problem hiding this comment.
You need to add the new seeds. And I need to update the csv with the new seeds ....
Collaborator
|
@SaulLu can you edit and merge? Thanks! |
Co-authored-by: Thomas Wang <24695242+thomasw21@users.noreply.github.com>
Co-authored-by: Thomas Wang <24695242+thomasw21@users.noreply.github.com>
for more information, see https://pre-commit.ci
Co-authored-by: Thomas Wang <24695242+thomasw21@users.noreply.github.com>
Contributor
Author
|
@HugoLaurencon, do you need it urgently ? |
Collaborator
|
@SaulLu No, no worries! It was just to tag you in case you didn't see the comments from Thomas |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
As discussed with @thomasw21, this PR add basic slurm and python scripts to compute an intermiadiary metadata dataset and some statistics for the Pseudo Crawl dataset